-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Optional Roving Tabindex to Toolbar #4557
base: main
Are you sure you want to change the base?
Conversation
const currentItem = this.controls[currentIndex][1]; | ||
currentItem.tabIndex = -1; | ||
|
||
let nextIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add type and initial value
@@ -133,9 +139,54 @@ class Toolbar extends Module<ToolbarProps> { | |||
} | |||
this.update(range); | |||
}); | |||
|
|||
if (this.hasRovingTabindex && input.tagName === 'BUTTON') { | |||
input.addEventListener('keydown', (e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanup required for eventListener?
var target = e.currentTarget; | ||
if (!target) return; | ||
|
||
switch (e.key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a switch with only one option is a bit strange, no?
// @ts-expect-error | ||
label.tabIndex = '0'; | ||
label.tabIndex = '-1'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this only happen for non roving or always?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now always set to -1 initially because I handle this in the setTabIndexes() method, which is called later once everything is initialized.
if (!target) return; | ||
|
||
switch (e.key) { | ||
case 'ArrowLeft': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, strange switch case
} | ||
|
||
handleKeyboardEvent(e: KeyboardEvent) { | ||
if (!this.hasRovingTabindex) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of duplicated. Can't you put the key handler on the toolbar or another parent of both buttons and picker?
This PR is not fully done yet, but will include an optional roving tabindex in the toolbar. This is a feature I need in order to have a better User Experience, so that users don't have to tab through each individual item. Rather, they tab into the toolbar, then the editor. Within the toolbar, they can navigate left and right with the ArrowKeys.